Skip to content
This repository has been archived by the owner on Jan 7, 2025. It is now read-only.

Ragdaemon7 #578

Closed
wants to merge 2 commits into from
Closed

Ragdaemon7 #578

wants to merge 2 commits into from

Conversation

granawkins
Copy link
Member

Pull Request Checklist

  • Documentation has been updated, or this change doesn't require that

Copy link
Contributor

@mentatai mentatai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request introduces several key changes, particularly in dependency updates and configuration handling. It's crucial to ensure that these changes are thoroughly tested, especially where they affect the system's core functionalities like file handling and daemon configuration. Overall, the updates seem to align with a general upgrade path but require careful consideration to avoid introducing new issues.

Butler is in closed beta. Reply with feedback or to ask Butler to review other parts of the PR. Please give feedback with emoji reacts.

@@ -38,7 +38,7 @@ async def run_auto_context_benchmark(sample: Sample, config: Config, cwd: Path |
ignore_patterns = [cwd / ".venv"]
annotators = {
"hierarchy": {"ignore_patterns": ignore_patterns},
"chunker_line": {"lines_per_chunk": 100},
"chunker": {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from chunker_line to chunker might affect how the data is processed. Can you confirm if this change was intended to align with updates in the ragdaemon package? It's important to ensure that this doesn't unintentionally alter the benchmarking process.

gitignore_contents = gitignore_path.read_text()
if ".ragdaemon/*" not in gitignore_contents:
gitignore_path.write_text(gitignore_contents + ".ragdaemon/*\n")

test_executable = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the .gitignore management for .ragdaemon might expose us to potential issues with untracked files getting into version control. Was there a specific reason for removing these lines? If it's about simplifying the setup, we might want to consider other ways to handle this, possibly at a higher level in the project configuration.

@@ -75,16 +71,14 @@ async def refresh_daemon(self):

annotators: dict[str, dict[str, Any]] = {
"hierarchy": {"ignore_patterns": [str(p) for p in self.ignore_patterns]},
"chunker_line": {"lines_per_chunk": 50},
"chunker": {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of graph_path and model from the Daemon initialization could lead to issues if the daemon relies on these parameters for specific functionalities. Please ensure that the daemon's new version is compatible with these changes and that no functionality is lost.

@@ -9,12 +9,12 @@ openai==1.13.3
pillow==10.1.0
prompt-toolkit==3.0.39
Pygments==2.15.1
ragdaemon==0.2.12
ragdaemon~=0.7.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating ragdaemon and spiceai packages could introduce breaking changes or deprecations. It's crucial to verify that all functionalities integrated with these packages are still compatible after the updates.

@@ -213,7 +213,7 @@ def func_4(string):
mock_session_context.config.auto_context_tokens = 8000

code_message = await code_context.get_code_message(0, prompt="prompt")
assert mock_session_context.llm_api_handler.spice.count_tokens(code_message, "gpt-4", is_message=True) == 85 # Code
assert mock_session_context.llm_api_handler.spice.count_tokens(code_message, "gpt-4", is_message=True) == 89 # Code
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expected token count has changed from 85 to 89. It's good to see that tests are updated to reflect changes in the system's behavior, but it would be beneficial to add a comment explaining why this change is expected for future reference.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants